@llvm/pr-subscribers-clangd

Author: Chris B (llvm-beanz)

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index a87da252b7a7e9..4136155403fc1d 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -623,6 +623,8 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
       {"clangdInlayHintsProvider", true},
       {"inlayHintProvider", true},
       {"foldingRangeProvider", true},
+      {"languages",
+       {"c", "cpp", "cuda-cpp", "objective-c", "objective-cpp", "hlsl"}},
   };
 
   {
diff --git a/clang-tools-extra/clangd/test/initialize-params.test b/clang-tools-extra/clangd/test/initialize-params.test
index a1fdae9870ab6e..81f00e0f469c52 100644
--- a/clang-tools-extra/clangd/test/initialize-params.test
+++ b/clang-tools-extra/clangd/test/initialize-params.test
@@ -48,6 +48,14 @@
 # CHECK-NEXT:      "implementationProvider": true,
 # CHECK-NEXT:      "inactiveRegionsProvider": true,
 # CHECK-NEXT:      "inlayHintProvider": true,
+# CHECK-NEXT:      "languages": [
+# CHECK-NEXT:        "c",
+# CHECK-NEXT:        "cpp",
+# CHECK-NEXT:        "cuda-cpp",
+# CHECK-NEXT:        "objective-c",
+# CHECK-NEXT:        "objective-cpp",
+# CHECK-NEXT:        "hlsl"
+# CHECK-NEXT:      ],
 # CHECK-NEXT:      "memoryUsageProvider": true,
 # CHECK-NEXT:      "referencesProvider": true,
 # CHECK-NEXT:      "renameProvider": true,

1. Since the key we're adding to the server capabilities is a non-standard extension, I'm a bit uneasy about giving it a general name like "languages". If the LSP were to add a key with that name but a different format or interpretation, we could run into trouble. Can we name it "clangdLanguages" instead, thereby staying within our own (improvised) "namespace"?

+1 This makes sense.

2. Given that the plan described by Sam in this comment is to enable vscode-clangd support for HLSL by default once clang's support for HLSL is a good enough state, what do you think about the following arrangement:
   • don't include "hlsl" in "clangdLanguages" yet
   • add "hlsl" to "clangdLanguages" once the "good enough" threshold is reached
   • on the client side, add HLSL to the document selector if the server announces support for "hlsl" in "clangdLanguages", or a client-side preference (which we could name "experimental", e.g. "clangd.experimentalHLSLSupport") is enabled?

What about a slightly different take? What if in addition to adding clangdLanguages (without HLSL) we also add clangdExperimentalFeatures and add HLSL there. The problem I'm trying to solve here is to avoid someone enabling HLSL on a language server that doesn't advertise that it has support.

That would still allow your points:

• early adopters can set "clangd.experimentalHLSLSupport" with an up-to-date client paired even with a clangd 17 or older server, and get the corresponding level of support
• once the implementation quality is good enough, the server can start announcing the support and all clients (not just early adopters who set the pref) will have it by default with newer server versions

What about a slightly different take? What if in addition to adding clangdLanguages (without HLSL) we also add clangdExperimentalFeatures and add HLSL there. The problem I'm trying to solve here is to avoid someone enabling HLSL on a language server that doesn't advertise that it has support.

That sounds fine to me!

ServerCapabilities has this member:

         /**
	 * Experimental server capabilities.
	 */
	experimental?: LSPAny;

Any reason we wouldn't just use that directly? maybe

  llvm::json::Object ServerCaps{
      ...
      {"experimental",
       llvm::json::Object{
           {"languages", {"HLSL"}},
       }},
      ...
  }

Using "ServerCapabilities.experimental" sounds reasonable to me.

I am afraid exposing this in server capabilities might be too convoluted.

IIUC, we're trying to figure out if clangd should be used for documents for language X. We need to figure this out before clangd's proper initialization, so that we can decide which documentSelectors to provide.

Hence spawning clangd with simple modes like --version etc. to figure out this information is OK, but I don't think we should pay the complexity of starting clangd, providing it with a initialize request and then parsing the output to decide and kill the language server afterwards (moreover, even if we did, I don't think this mechanism would be meaningful/usable by any other LSP client).

IIUC, we're trying to figure out if clangd should be used for documents for language X. We need to figure this out before clangd's proper initialization, so that we can decide which documentSelectors to provide.

Thanks, that's a good point that I overlooked!

(I think for some reason I imagined the document selector would be a function called at the time a file is opened, such that we can feed information from the actual clangd instance's server capabilities into it, and use that information for any file except the first one that triggered clangd's launch. But I see that it's actually just a declarative list of filters that needs to be provided up front.)

So, @llvm-beanz, I think we may need to abandon this idea, and go back to the drawing board on clangd/vscode-clangd#392. We can start with enabling HLSL support based just on the presence of the client-side config option. Then once we're looking to make it the default, we could look at parsing the output of clangd --version as Kadir suggested and gate it on that.